-
Couldn't load subscription status.
- Fork 123
[FileBasedConfiguration] Resources #4462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FileBasedConfiguration] Resources #4462
Conversation
> Co-authored-by: Yevhenii Solomchenko <[email protected]>
33e7e00 to
77bc6b7
Compare
it is correctly handled by OTel SDK
de21a27 to
91d64bb
Compare
detection of these attributes was moved to resourceconfigurator
src/OpenTelemetry.AutoInstrumentation/Configurations/ResourceSettings.cs
Outdated
Show resolved
Hide resolved
|
Will you address the TODOs here or later? |
src/OpenTelemetry.AutoInstrumentation/Configurations/FileBasedConfiguration/Parser/Parser.cs
Show resolved
Hide resolved
src/OpenTelemetry.AutoInstrumentation/Configurations/ResourceSettings.cs
Outdated
Show resolved
Hide resolved
Follow up PRs. I would like to avoid making this PR bigger. |
src/OpenTelemetry.AutoInstrumentation/Configurations/ResourceSettings.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.AutoInstrumentation/Configurations/ResourceSettings.cs
Outdated
Show resolved
Hide resolved
...Telemetry.AutoInstrumentation/Configurations/FileBasedConfiguration/ResourceConfiguration.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general seems fine as follow ups seem to come anyway and fix some of the parts per time.
...Telemetry.AutoInstrumentation/Configurations/FileBasedConfiguration/ResourceConfiguration.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Gets or sets the list of enabled resources. | ||
| /// </summary> | ||
| public IReadOnlyList<KeyValuePair<string, object>> Resources { get; set; } = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the future, may want to use union ref struct not to cause object boxing for simple types.
src/OpenTelemetry.AutoInstrumentation/Configurations/ConfigurationKeys.cs
Show resolved
Hide resolved
| protected virtual void OnLoadFile(YamlConfiguration configuration) | ||
| { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method should also be abstract in the future. It’s worth adding a TODO: that once the implementation is done in all settings, it should be changed to abstract.
Co-authored-by: Yevhenii Solomchenko <[email protected]>
|
@rajkumar-rangaraj, @nrcventura, @zacharycmontoya. I am going to merge to unblock further work on this tasks and NoCode feature. |
Why
Towards #4394
Extracted and refactored from #4270
What
File based configuration for resources.
Note
File based configuration should block any direct reads from environmental variables. In this case, it means that Environmental Variable resource detector is not used.
Intentionally keeping documentation and changelog outside scope of this PR. I think that for documentation we need at least one more PR (resource detectors).
Changelog should be mentioned only when we decide ask the end-users to test it.
Tests
CI + added tests
Checklist
[ ]CHANGELOG.mdis updated.[ ] Documentation is updated.